Repository navigation
Don't re-lock the sender mutex when #add can't restart the thread - #335
JakeHuneau wants to merge 2 commits into
Conversation
#add calls #start while holding @mx. When creating the thread raised ThreadError, the rescue in #start locked @mx again and Ruby raised "deadlock; recursive locking". Set @done directly when the current thread already owns the mutex, and stop #add from queuing a message that no thread will send. Fixes DataDog#333
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c34635c62
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| start | ||
| # the companion thread could not be re-created, don't queue | ||
| # messages that nothing will ever send | ||
| return if @done |
There was a problem hiding this comment.
When Thread.new fails during the restart and the caller subsequently invokes the public Statsd#sync_with_outbound_io, this return leaves the queue allocated by start non-nil even though it has no consumer. Forwarder#sync_with_outbound_io calls Sender#rendez_vous, whose message_queue guard therefore passes before it enqueues a reply queue and blocks on queue.pop forever; because this patch now hides the ThreadError and lets the caller continue, the failed-start state needs to be visible to rendez_vous (or that method must also check @done/the worker).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, that's reachable once #add no longer raises. Fixed in a9df8e1: #rendez_vous now returns unless the sender thread is alive, the same check #flush already makes, so sync_with_outbound_io doesn't block on a queue nobody reads. Added a spec that calls rendez_vous after the failed restart under a 1 s timeout; it times out without the fix.
After a failed restart the message queue exists but no thread reads it, so #rendez_vous (and Statsd#sync_with_outbound_io) blocked forever on the reply queue. Return early unless the sender thread is alive, as #flush already does.
Fixes #333.
Sender#addcalls#startwhile holding@mxwhen the companion thread is dead. If creating the new thread raisesThreadError(for example while the process is exiting), the rescue in#startran@mx.synchronize { @done = true }on a mutex the current thread already owned, so Ruby raisedThreadError: deadlock; recursive locking. That is the #306 scenario again, with a different message.#startnow sets@donedirectly when@mx.owned?, and still takes the lock otherwise (the Rescue ThreadError when creating Sender Thread #307 case, where#startis called on its own).#addreturns after a failed restart, instead of pushing the message onto the new queue that no thread reads. Later calls already return early because@doneis set.#rendez_vousreturns unless the sender thread is alive, as#flushalready does. Without that,Statsd#sync_with_outbound_ioafter a failed restart would wait forever on a queue no thread reads.The new specs start the sender with a thread that has already finished, then make
Thread.newraise; the first calls#addtwice. A second spec calls#rendez_vousin that state under a timeout. Onmasterthe first fails withdeadlock; recursive locking; with this change all 662 examples pass (Ruby 3.4).Mutex#owned?is available since Ruby 2.0, so it works on all the Rubies in the CI matrix.